JAMES-4210 Generic SASL mechanism extension#3059
Conversation
quantranhong1999
force-pushed
the
sasl-modularize
branch
from
14ba04f to
cb204cf
Compare
|
Hello, I did try a lot to make the POC ready so far, for IMAP. I did rebase on the latest master to have the You can just jump to the last commit JAMES-4210 Add custom IMAP SASL extension example to see how we could implement an SASL mechanism extension for IMAP. Note that I did rush to make this POC alive for IMAP, so of course, review + polish and double-check would be needed later :) |
There was a problem hiding this comment.
I'm glad that we get something more or less working, that's an achievement in itself. Koodo.
Now remain the next step: SIMPLIFY ! ANd rationalize... I'd expect this proposal to be refined so that we drop the extra aaccidental complexity.
Expect the process to take several working day (and to be painful) but we will be happy I believe about the end result!
I'd suggest we rationnalize before extending to other protocols. I'd expect from this protocol an actual code reduction by switching from O(SxP) implementation complexity to O(S+P) complexity where S is the count of SASL mechanism and P the count of protocols - what this implementation do not deliver yet !
Arsnael
left a comment
Arsnael
left a comment
There was a problem hiding this comment.
Read it, not much to add for now, just minor comments
quantranhong1999
changed the title
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Sorry to be picky, but this work shall be simplified further.
Let's invest more time on it.
Hello, I did attempt to:
|
| * | ||
| * @param channelEncrypted whether the underlying transport is encrypted, for example with TLS. | ||
| */ | ||
| default boolean isAvailableOnTransport(boolean channelEncrypted) { |
There was a problem hiding this comment.
We can concider passin a raw ProtocolSession here I think if we want to be more generic
There was a problem hiding this comment.
ImapSession does not extend ProtocolSession, therefore ProtocolSession is not generic enough to be used here...
Unless we refactor ImapSession to extend ProtocolSession, which feels like another core refactoring.
I guess we can live with boolean channelEncrypted for now, and refactor it further later if needed.
| private final Authorizator authorizator; | ||
|
|
||
| @Inject | ||
| public JamesSaslAuthenticator(Authenticator authenticator, Authorizator authorizator) { |
There was a problem hiding this comment.
Or can't we rather user SessionProvider ?
How about letting the Sasl mechanism directly play with SessionProvider ? And we just inject SessionProvider within SaslMechanismFacoties ?
There was a problem hiding this comment.
IMO, we should avoid injecting SessionProvider directly into SASL mechanisms because SessionProvider returns MailboxSession, which is an IMAP/mailbox-specific session object.
For example, if SMTP later reuses OAUTHBEARER, the mechanism should not create a MailboxSession. SMTP only needs the authenticated/authorized identity to update its own SMTP session state and relay/submission policy.
That is why I prefer keeping a small SaslAuthenticator wrapper: mechanisms perform credential validation and authorization, then return a protocol-neutral SaslAuthenticationResult.Success(SaslIdentity). Each protocol remains responsible for initializing its own session model from that identity.
There was a problem hiding this comment.
I do not understand why we cannot unify this behaviour as well TBH
|
I will plan SMTP adoption in another PR. We can wait to see whether SMTP adoption is well aligned and viable following the SPI introduced, before merging this IMAP work, if needed. But I presume SMTP adoption would be good. |
Yes please |
Add the shared SASL exchange model: mechanism factories, initial requests, exchange lifecycle, typed steps, identities, authentication results, and failures. Add contract tests documenting one-step, multi-step, delegation, payload copy, and lifecycle behavior.
Add protocols-sasl with James-backed PLAIN, OAUTHBEARER, and XOAUTH2 factories. Move password and token validation into mechanisms through JamesSaslAuthenticator, keep OIDC config per server block, and add unit coverage for PLAIN and OIDC behavior.
Add shared Guice resolution for auth.saslMechanisms, supporting built-in simple names, external FQCNs, server-specific factory creation, duplicate mechanism deduplication, and configured order preservation.
Drive IMAP AUTHENTICATE and LOGIN through the new SASL exchange model. Add IMAP SASL bridge encoding and decoding, final server-data handling, exchange cleanup, typed failure mapping, and preserve mailbox session semantics including delegation.
Resolve SASL mechanisms per IMAP server configuration in Guice, keep default factory override support for custom IMAP packages, and ensure capability and enable processors belong to the same IMAP processor suite.
Remove authentication and OIDC configuration from IMAPServer, ImapSession, and NettyImapSession; SASL configuration now belongs to the mechanism factory layer. Adapt default and Spring IMAP processor creation so JPA/Spring and tests still get proper default mechanisms.
Update the custom IMAP example to provide a configurable SASL mechanism factory through auth.saslMechanisms, covering SASL-IR, continuation, invalid token, and final server-data flows.
Add WebAdmin regression coverage proving IMAP SASL authentication preserves loggedInUser, including delegated authentication where the connected username and logged-in user differ.
quantranhong1999
force-pushed
the
sasl-modularize
branch
from
b30dd7f to
b2179c7
Compare
|
Hello, I just rebased and reworked the commit history. The changes are the same as before. |
|
Green build. Objections to merging this? |
|
No objection on my side given that we work on a timely manner on Pop3 and manageSieve |
3 participants
TODO: